fix: prevent ForeignKeyViolation error on delete #23414

betodealmeida · 2023-03-17T21:02:07Z

SUMMARY

We've been seeing sporadic non-reproducible ForeignKeyViolation errors in our logs when users try to delete charts:

psycopg2.errors.ForeignKeyViolation: update or delete on table "slices" violates foreign key constraint "slice_user_slice_id_fkey" on table "slice_user"
DETAIL:  Key (id)=(68) is still referenced from table "slice_user".

And datasets:

psycopg2.errors.ForeignKeyViolation: update or delete on table "tables" violates foreign key constraint "sqlatable_user_table_id_fkey" on table "sqlatable_user" 
DETAIL:  Key (id)=(18) is still referenced from table "sqlatable_user".

Looking at the docs this shouldn't happen for secondary associations, though other people have reported the same problem. In order to fix it I changed the delete commands to manually clear the owners attribute, and also added an ondelete clause so that the database handles the cascade (in addition to SQLAlchemy).

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2023-03-17T21:04:19Z

Codecov Report

Merging #23414 (f3bea7f) into master (d950eb8) will increase coverage by 0.02%.
The diff coverage is 86.85%.

❗ Current head f3bea7f differs from pull request most recent head 2d9abb0. Consider uploading reports for the commit 2d9abb0 to get more accurate results

@@            Coverage Diff             @@
##           master   #23414      +/-   ##
==========================================
+ Coverage   67.61%   67.63%   +0.02%     
==========================================
  Files        1907     1908       +1     
  Lines       73590    73733     +143     
  Branches     7982     7989       +7     
==========================================
+ Hits        49755    49871     +116     
- Misses      21787    21814      +27     
  Partials     2048     2048

Flag	Coverage Δ
hive	`52.72% <45.16%> (-0.02%)`	⬇️
mysql	`78.44% <81.29%> (+0.03%)`	⬆️
postgres	`78.50% <81.29%> (+<0.01%)`	⬆️
presto	`52.65% <45.80%> (-0.02%)`	⬇️
python	`82.36% <89.67%> (+0.04%)`	⬆️
sqlite	`77.00% <80.00%> (+0.03%)`	⬆️
unit	`52.58% <57.41%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ackages/superset-ui-core/src/utils/featureFlags.ts	`100.00% <ø> (ø)`
...s/legacy-plugin-chart-country-map/src/countries.ts	`100.00% <ø> (ø)`
...chart-echarts/src/Timeseries/Area/controlPanel.tsx	`40.00% <ø> (ø)`
...charts/src/Timeseries/Regular/Bar/controlPanel.tsx	`35.71% <ø> (ø)`
...gin-chart-echarts/src/components/ExtraControls.tsx	`0.00% <0.00%> (ø)`
...d/plugins/plugin-chart-echarts/src/utils/series.ts	`88.48% <0.00%> (+1.43%)`	⬆️
superset-frontend/src/SqlLab/reducers/sqlLab.js	`36.87% <ø> (ø)`
...c/components/Chart/DrillDetail/DrillDetailPane.tsx	`75.00% <ø> (ø)`
...tersConfigModal/FiltersConfigForm/ColumnSelect.tsx	`77.14% <ø> (ø)`
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx	`55.17% <ø> (-4.49%)`	⬇️
... and 31 more

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

betodealmeida · 2023-03-20T20:40:34Z

/testenv up

github-actions · 2023-03-20T20:42:37Z

@betodealmeida Ephemeral environment spinning up at http://35.87.238.215:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

superset/models/slice.py

github-actions · 2023-03-21T22:49:15Z

Ephemeral environment shutdown and build artifacts deleted.

zephyring · 2023-03-22T01:05:12Z

superset/charts/commands/delete.py

@@ -45,8 +45,13 @@ def __init__(self, model_id: int):

    def run(self) -> Model:
        self.validate()
+        self._model = cast(Slice, self._model)


is this necessary? The doc said This returns the value unchanged. but only good for type checker

It's needed to make the type checker happy.

On __init__ self._model will be set to None. When we call self.validate() it will either set self._model to a Slice or raise an exception, so here we're telling the type checker that we know self._model is of type Slice.

tooptoop4 · 2023-04-03T06:12:48Z

i'm facing this issue in 2.1.0 rc3

santosh-sahoo334 · 2023-05-02T10:24:10Z

I'm facing this issue in 2.1.0. This version is still unstable ?

fix: prevent ForeignKeyViolation error on delete

d1a3fb9

pull-request-size bot added the size/S label Mar 17, 2023

betodealmeida force-pushed the sc62626 branch from 546dfd0 to fe531a9 Compare March 17, 2023 21:27

Fix type

2f756c7

betodealmeida force-pushed the sc62626 branch from fe531a9 to 2f756c7 Compare March 17, 2023 21:28

betodealmeida requested a review from dpgaspar March 20, 2023 23:39

dpgaspar reviewed Mar 21, 2023

View reviewed changes

superset/models/slice.py Outdated Show resolved Hide resolved

Remove ondelete

2d9abb0

betodealmeida requested a review from dpgaspar March 21, 2023 18:47

hughhhh self-requested a review March 21, 2023 22:46

hughhhh approved these changes Mar 21, 2023

View reviewed changes

betodealmeida merged commit 45f045d into master Mar 21, 2023

zephyring reviewed Mar 22, 2023

View reviewed changes

john-bodley mentioned this pull request Jun 22, 2023

chore(dao): Add explicit ON DELETE CASCADE when deleting datasets #24488

Merged

9 tasks

laxmanraparthi mentioned this pull request Nov 26, 2023

[Snyk] Fix for 10 vulnerabilities ShareChat/superset#56

Open

matrix-compute mentioned this pull request Dec 14, 2023

[Snyk] Fix for 15 vulnerabilities matrix-compute/superset#13

Open

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024

mistercrunch deleted the sc62626 branch March 26, 2024 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prevent ForeignKeyViolation error on delete #23414

fix: prevent ForeignKeyViolation error on delete #23414

betodealmeida commented Mar 17, 2023 •

edited by john-bodley

Loading

codecov bot commented Mar 17, 2023 •

edited

Loading

betodealmeida commented Mar 20, 2023

github-actions bot commented Mar 20, 2023

github-actions bot commented Mar 21, 2023

zephyring Mar 22, 2023

betodealmeida Mar 22, 2023 •

edited

Loading

tooptoop4 commented Apr 3, 2023

santosh-sahoo334 commented May 2, 2023

fix: prevent ForeignKeyViolation error on delete #23414

fix: prevent ForeignKeyViolation error on delete #23414

Conversation

betodealmeida commented Mar 17, 2023 • edited by john-bodley Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Mar 17, 2023 • edited Loading

Codecov Report

betodealmeida commented Mar 20, 2023

github-actions bot commented Mar 20, 2023

github-actions bot commented Mar 21, 2023

zephyring Mar 22, 2023

Choose a reason for hiding this comment

betodealmeida Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

tooptoop4 commented Apr 3, 2023

santosh-sahoo334 commented May 2, 2023

betodealmeida commented Mar 17, 2023 •

edited by john-bodley

Loading

codecov bot commented Mar 17, 2023 •

edited

Loading

betodealmeida Mar 22, 2023 •

edited

Loading